permission: support URL and Uint8Array as has()/drop() reference - #65492
Conversation
|
Review requested:
|
Codecov Reportβ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65492 +/- ##
==========================================
+ Coverage 90.11% 90.14% +0.03%
==========================================
Files 752 751 -1
Lines 251861 252647 +786
Branches 47365 47549 +184
==========================================
+ Hits 226955 227745 +790
+ Misses 16238 16192 -46
- Partials 8668 8710 +42
π New features to boost your workflow:
|
6d518fc to
72717a5
Compare
normalizeReference() adds URL and Uint8Array support to has()/drop(); the existing string/Buffer behavior is unchanged. BufferValue replaces Utf8Value so a Buffer/TypedArray reference is copied as raw bytes instead of a lossy UTF-8 conversion. Signed-off-by: seungmin Nam <nhjbest22@g.skku.edu>
72717a5 to
42241f8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Thank you so much for constantly keeping an eye on my PR and explaining the CI status! I'm truly amazed by all the care and effort you've put into this. Based on your explanation, it sounds like we might see a green CI soon! I'll sit tight and wait a bit longer. |
This comment was marked as outdated.
This comment was marked as outdated.
I didn't expect to be spending so much effort surrounding your PR, however it has got caught up in several generic issues, so that's why I'm continuing to follow it and get it resolved together with others on the team. So far none of the issues seem to come from changes you've proposed in this PR, so I am hopeful to see a green CI soon! Thanks for your patience! |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
|
Jenkins is now green after many attempts π. The issues it found had nothing to do with the contents of this PR and these are hidden now as resolved / outdated in the history. Marking as
author ready
|
|
Thank you so much for all your hard work and dedication to getting the CI to pass. The effort you put into this means a lot to an early contributor like me. I look forward to continuing to contribute and working together for the Node.js community. Thanks again! |
|
Thanks for the support @MikeMcC399 |
|
Landed in c4336d9 |
|
I'm happy to see that your PR has successfully landed! If you continue contributing, I do hope you have a better experience next time. In any case, now that you are a recognized Contributor you will find that the GitHub Actions CI runs automatically, instead of requiring an individual approval for every change. The Jenkins CI does however always need a Collaborator to first review a PR and then start the Jenkins CI. There were a lot of different problems that came to together during the same period, including GitHub's own issues. I have to thank also @sxa and others from the team for their assistance in the background! |
|
Timing has not been kind to this PR π ! 2ec0f9a which landed at 13:07 UTC has broken tests in the The tests after merging this PR c4336d9 at 14:59 UTC that have failed. A PR #65683 is already queued to fix the tests. |
process.permission.has()/drop() only accepted a string or Buffer for
the
referenceargument. This adds support for a WHATWG URL, resolvedvia fileURLToPath() for fs.* scopes since those are the only scopes
that actually use the reference value, and a plain Uint8Array.
Also switches permission.cc from Utf8Value to BufferValue when reading
a Buffer/TypedArray reference, since Utf8Value forces a UTF-8 string
conversion that can silently corrupt a path that isn't valid UTF-8.